Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to v0.4.0 #94

Open
wants to merge 40 commits into
base: master
Choose a base branch
from
Open

Upgrade to v0.4.0 #94

wants to merge 40 commits into from

Conversation

TakodaS
Copy link

@TakodaS TakodaS commented Sep 7, 2023

Description

Updated arkworks-rs dependencies to v0.4.0.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests (Not needed as no new functionality was added).
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@TakodaS TakodaS marked this pull request as draft September 7, 2023 14:41
@TakodaS
Copy link
Author

TakodaS commented Sep 7, 2023

@Pratyush Should I bother implementing CryptographicSponge for the Fiat-Shamir hash function found in rng or should I rip this all out in favour of Poseidon?

Edit: I see something similar is already in PR. Maybe I can commandeer that PR. I will go talk to @vlopes11.

@TakodaS TakodaS marked this pull request as ready for review September 7, 2023 16:15
@TakodaS TakodaS changed the title Draft: Upgrade to v0.4.0 Upgrade to v0.4.0 Sep 7, 2023
@TakodaS
Copy link
Author

TakodaS commented Sep 12, 2023

@Pratyush Should I bother implementing CryptographicSponge for the Fiat-Shamir hash function found in rng or should I rip this all out in favour of Poseidon?

Edit: I see something similar is already in PR. Maybe I can commandeer that PR. I will go talk to @vlopes11.

Implementing FS hashing using the PoseidonSponge, temporarily with the same default initialization as used in the tests for crypto-primitives.


rayon = { version = "1", optional = true }
digest = { version = "0.9" }
derivative = { version = "2", features = ["use_core"] }
itertools = "0.11.0"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will probably have to remove this to pass the no-std test.

#[derive(Clone)]
pub struct SimplePoseidonRng<F: PrimeField>(PoseidonSponge<F>);

impl<F: PrimeField> RngCore for SimplePoseidonRng<F> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RngCore is required for backwards compatibility with generation of random field elements in ark-ff and ark-poly. Changing these is a much larger piece of work than exposing the sponge construction through RngCore and beyond the scope of this PR imo.

@TakodaS TakodaS requested a review from Pratyush September 28, 2023 11:05
Comment on lines +67 to +89
/// Instantiate Poseidon sponge with default parameters
impl<F: PrimeField> Default for SimplePoseidonRng<F> {
fn default() -> Self {
// let default =
// Self(PoseidonSponge::new(&poseidon_parameters_for_test()))
let (alpha, rate, full_rounds, partial_rounds) = (17, 2, 8, 29);
let (ark, mds) = find_poseidon_ark_and_mds(
F::MODULUS_BIT_SIZE as u64,
rate,
full_rounds,
partial_rounds,
0,
);
let config = PoseidonConfig {
full_rounds: full_rounds as usize,
partial_rounds: partial_rounds as usize,
alpha: alpha as u64,
ark,
mds,
rate,
capacity: 1,
};
SimplePoseidonRng(PoseidonSponge::new(&config))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope these are good defaults.

src/lib.rs Outdated
impl<F: PrimeField, PC: PolynomialCommitment<F, DensePolynomial<F>>, FS: FiatShamirRng>
Marlin<F, PC, FS>
impl<
F: PrimeField + Absorb,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of the Absorb trait bound is very problematic. Although it will improve efficiency of absorption, it makes the implementation far less generic. I am not certain how to improve this, considering that specialization of traits is an unstable feature.

Copy link
Author

@TakodaS TakodaS Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I resolved this by implementing the fast_prove and fast_verify methods that may be invoked when the PrimeField also satisfies the Absorb trait bound. I think this is a good trade off, since Absorb is not nearly as well implemented as Canonical(De)Serialize and it may be a pain for people to upgrade to the new implementation. Over time, I think the old method can be deprecated in favor of the new, but I think a prerequisite is implementing a derive Absorb macro.

Obviously there is a lot of code duplication, this can be refactored if @Pratyush approves the design philosophy here.

@autquis
Copy link

autquis commented Feb 13, 2024

Any updates on this? I am ready to take any necessary steps to make it ready to merge. @Pratyush @mmagician

Comment on lines +380 to +385
let (eta_a, eta_b, eta_c) = rng
.squeeze_field_elements(3)
.iter()
.map(|x: &F| x.to_owned())
.collect_tuple()
.unwrap();
Copy link
Member

@Pratyush Pratyush Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let (eta_a, eta_b, eta_c) = rng
.squeeze_field_elements(3)
.iter()
.map(|x: &F| x.to_owned())
.collect_tuple()
.unwrap();
let [eta_a, eta_b, eta_c] = rng
.squeeze_field_elements(3)[..3]
else { unreachable!("should have three elements") };

Comment on lines +317 to +322
let (f1, f2, f3) = rng
.squeeze_field_elements(3)
.iter()
.map(|x: &F| x.to_owned())
.collect_tuple()
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not secure; we cannot use the sponge for randomness for zero knowledge. The old version should work fine.

Comment on lines +62 to +67
let (eta_a, eta_b, eta_c) = rng
.squeeze_field_elements(3)
.iter()
.map(|x: &F| x.to_owned())
.collect_tuple()
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let (eta_a, eta_b, eta_c) = rng
.squeeze_field_elements(3)
.iter()
.map(|x: &F| x.to_owned())
.collect_tuple()
.unwrap();
let [eta_a, eta_b, eta_c, ..] = rng
.squeeze_field_elements(3)[..3]
else { unreachable!("should be of size 3) };

Comment on lines +200 to +210
let fcinput = first_comms
.iter()
.map(|p| p.commitment().clone())
.collect::<Vec<_>>();

fs_rng.absorb(&to_bytes![first_comms, prover_first_msg].unwrap());
match prover_first_msg {
ProverMsg::FieldElements(ref elems) => {
absorb!(&mut fs_rng, &to_bytes(&fcinput), &to_bytes(elems));
}
ProverMsg::EmptyMessage => fs_rng.absorb(&to_bytes(&fcinput)),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wrap this into a helper function.

Comment on lines +231 to +240
let scinput = second_comms
.iter()
.map(|p| p.commitment().clone())
.collect::<Vec<_>>();
match prover_second_msg {
ProverMsg::FieldElements(ref elems) => {
absorb!(&mut fs_rng, &to_bytes(&scinput), &to_bytes(elems));
}
ProverMsg::EmptyMessage => fs_rng.absorb(&to_bytes(&scinput)),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Comment on lines +260 to +269
let tcinput = third_comms
.iter()
.map(|p| p.commitment().clone())
.collect::<Vec<_>>();
match prover_third_msg {
ProverMsg::FieldElements(ref elems) => {
absorb!(&mut fs_rng, &to_bytes(&tcinput), &to_bytes(elems));
}
ProverMsg::EmptyMessage => fs_rng.absorb(&to_bytes(&tcinput)),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

Comment on lines +396 to +403
match &proof.prover_messages[0] {
ProverMsg::FieldElements(ref elems) => {
absorb!(&mut fs_rng, &to_bytes(first_comms), &to_bytes(elems));
}
ProverMsg::EmptyMessage => fs_rng.absorb(&to_bytes(first_comms)),
}
let (_, verifier_state) =
AHPForR1CS::verifier_first_round(index_vk.index_info, &mut fs_rng)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the helper function here also.

{
/// Create a zkSNARK asserting that the constraint system is satisfied.
/// Uses fast absorption of field elements into sponge
pub fn fast_prove<C: ConstraintSynthesizer<F>, R: RngCore + CryptographicSponge>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this new function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +141 to +146
let (a, b) = rng
.squeeze_field_elements(2)
.iter()
.map(|x: &Fr| x.to_owned())
.collect_tuple()
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create a helper squeeze macro.

@TakodaS
Copy link
Author

TakodaS commented Mar 6, 2024

Any updates on this? I am ready to take any necessary steps to make it ready to merge. @Pratyush @mmagician

Go right ahead! I guess you can make a new pull request and I will close this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants